jit: read a deref-last projection as a value, and put fast2locals on the virtualizable protocol - #1358
Conversation
`build_rvalue`'s `Rvalue::Ref` and `Rvalue::RawPtr` arms called every `PlaceKind::Projection` an address-of. `&(*p).f` names the field's address; `&*(*p).f` reads the pointer the field holds and dereferences it. Both are projections, so both were marked. `mark_place_address_of` sets `taken_by_address` on the descriptor of the last op the projection emitted, which is the `getfield`. `rewrite_op_getfield` folds `suppresses_virtualizable()` into `fresh_virtualizable`, and the `vable_array_vars` insert is gated on `!fresh_virtualizable`, so a marked read is never registered: it stays an ordinary `getarrayitem_gc` against the heap array, which is written back only at `sync_virtualizable_before_jit`, `sync_virtualizable_after_jit` and `sync_virtualizable_after_guard_failure`. `locals_w!` expands to `&*$frame.locals_cells_stack_w`, the deref-last shape, and it has 44 call sites across `pyframe.rs`, `eval.rs` and `builtins.rs`. `place_ref_is_address_of` spells the `Deref` test as `resolve_place` and `emit_projection_write` already spell it, applied one level out. `address_of_the_vable_array_slot_is_marked_not_a_read` keeps the `addr_of_mut!` shape, whose outermost step is a field, on the marked side. Assisted-by: Claude
A subscript evaluates its receiver before its index expression, so `locals_w!(self)[self.valuestackdepth - 1 - depth]` emits the `locals_cells_stack_w` read first and the subtraction's overflow check after it. That check branches, so the array is defined in one block and consumed in another. Lowering `peek_at` shows it directly: the `FieldRead` is in bb0, the two `sub`s split bb0 from bb4 from bb5, and the `ArrayRead` is in bb5, with the array carried as a link argument the whole way. `_check_no_vable_array` rejects that — `Escaped via: link argument` — and the rejection is a `panic!` inside `generate_into`, which `pyre-jit-trace`'s `build.rs` calls with no `catch_unwind`, so it is a build failure rather than a degraded trace. The panic message names this cause first: "indexing with an index not known non-negative". `peekvalue_maybe_none` and `settopvalue` already compute the index into a local before the subscript, which is also how `pyframe.py:479-484` spells it, and lowering the former puts its `FieldRead` and `ArrayRead` in one block. Six sites did not: `peek`, `peek_at`, `peekvalues` (`base + idx`) and the three reads in `with_except_start`. The slice bounds check does not split a block — the front folds it into the `ArrayRead` — only the arithmetic does. Assisted-by: Claude
`pyframe.py:572` decorates `fast2locals` `@jit.unroll_safe`. `look_inside_graph` cancels `contains_loop` for a graph carrying the hint (`policy.py:61-62`), so the slot loop no longer keeps the codewriter out. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe MIR translator now distinguishes address-taking projections from value reads. The interpreter also hoists stack-index calculations before virtualizable array reads and marks ChangesMIR address classification
JIT stack access
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to This PR changes JIT handling of frame-local reads and enables optimized frame synchronization paths. The package-specific verification passes, but the required dynasm-enabled repository checks are still outstanding, so merge readiness is moderate until those checks pass or the owner explicitly accepts the gap. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11925bcec7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// allocates a fresh dict (pyframe.py:557 `self.space.newdict(instance=True)`) | ||
| /// and caches it, so `locals() is locals()` holds. Errors propagate. | ||
| /// | ||
| /// `@jit.unroll_safe` (`pyframe.py:572`) cancels `contains_loop` in the |
There was a problem hiding this comment.
Cite the fast2locals decorator by symbol
In the checked-in pypy/interpreter/pyframe.py, line 572 is the cell.get() call, not the @jit.unroll_safe decorator; the decorator belongs to PyFrame.fast2locals and is currently at line 539. Replace this stale line reference with a symbol-based citation such as pypy/interpreter/pyframe.py PyFrame.fast2locals, as required by the root AGENTS.md, so the comment points to the evidence it claims.
AGENTS.md reference: AGENTS.md:L188-L191
Useful? React with 👍 / 👎.
…e three #1358 follow-ups (#1374) * jit: name a virtualizable config whose array lengths were never patched in `to_optimizer_config` builds `VirtualizableConfig` with `array_lengths: vec![]` and relies on its caller to fill them in: `MetaInterp::current_virtualizable_optimizer_config` assigns `ctx.virtualizable_array_lengths()` one line later, beside the identical patch of `vable_input_offset`. That sibling field documents the convention on itself; `array_lengths` did not. A length is not a property of the shape — upstream reads `len(lst)` off the live object in every `virtualizable.py` accessor and stores it nowhere. `VirtualizableTracker::init` zips `array_field_offsets` with `array_lengths`, so a config that declares an array field and carries no length runs that loop zero times, leaves `state.arrays` empty, and turns every later `tracked_array_element` into a miss that reads as "this trace had no array elements". The `debug_assert!` names that state instead of absorbing it. Both escapes in the assertion are load-bearing: the state-field macro JIT sets `track_array_elements = false` and carries its elements through the live `virtualizable_boxes` shadow, and a virtualizable with no array field has nothing to seed. `array_tracking_config_without_lengths_is_named_not_absorbed` has to build the state by hand, which is itself the statement that no production path produces it: both writers of `TraceCtx::virtualizable_boxes` set the lengths in the same statement, `state.rs seed_virtualizable_boxes` passes `vec![array_len]` on the portal and bridge paths, and `optimizer_vable_config_matches_registered_virtualizable_when_boxes_active` already pins the patched result. Assisted-by: Claude * interpreter: record why _unpackiterable_known_length_jitlook carries no unroll_safe The doc comment quotes upstream's `@jit.unroll_safe` along with the body it ports, which reads as an unfinished port. It is not one. Upstream reaches that body two ways and hints only one of them: `unpackiterable` goes through `_unpackiterable_known_length`, which is `@jit.dont_look_inside` ("the JIT stopped looking inside already"), while `unpackiterable_unroll` calls it directly with an UNPACK_SEQUENCE oparg as `expected_length`. pyre has neither `unpackiterable_unroll` nor the shim, so `unpackiterable` is this body's only caller — the one upstream fences off. Being loopy and unhinted the graph is rejected by `look_inside_graph` and stays a residual call, which is the boundary the shim buys upstream. Carrying the attribute alone would open that path, with `expected_length` — a red argument on one graph ~40 callers share — as the unroll bound. Restoring the split needs more than the attribute: `#[majit_macros::dont_look_inside]` registers a helper call descriptor, and `helper_call_kind_for_type` answers `Unsupported` for this signature's `Result<Vec<PyObjectRef>, PyError>`, so the shim needs an `extern "C" fn(..) -> i64` publication first. `test_unroll_safe_inventory` asserts the harvested `unroll_safe` set is a subset of a reviewed list, plus a named negative for this body. Subset rather than equality because a developer's `build/llbc` is routinely older than the source and can only under-report, which must not red; `builtins:: leading_non_null_count` is the positive control, and its absence skips the test loudly rather than passing on an artefact too old to say anything. Assisted-by: Claude * jit: carry the iterator element type into the next fold `iter_next_item_type` answered `Int` for a container produced by `front::range_iter`'s `range()` builtin and `Ref` for every other one. The `iter` op carries the iterator, not the container's item type, and a slice of non-GC items is spelled exactly like a slice of references — `is_concrete_iter_constructor` collapses `Vec<T>`, `[T; N]` and `Box<[T]>` onto the same `core::slice::…::iter`. So the container alone could not separate them. `charon-corpus`'s `branch_loop_sum(slice: &[i64], ..)` folds `for &v in slice`, and its `i64` element was typed as a GC reference. `result_ty` is not a hint the rtyper overrules: `resolve_call_result_kind` consults `concretetype` only when `result_ty` is `Unknown`, and `authoritative_result_types` stamps the derived kind back over it, so the answer here outranks the rtyper for every graph that gets a JitCode. The recording site already reads a callee's `Result` payload for `result_exc_call_results`; `next_call_results` now carries the `Option<T>` payload the same way, with the `&` a slice iterator adds peeled off by `strip_ty_wrappers`. `Ref(Some(root))` normalises back to `Ref(None)` so every GC-element graph that folds today stamps a byte-identical `result_ty`, and the range arm answers before the recorded type is consulted, because `rrange.py ll_rangenext_*` returns `Signed` whatever the Rust range spells. An unreadable `Option` shape falls back to `Ref(None)`, the answer the fold assumed unconditionally before. `branch_loop_sum_next_yields_an_int_element` fails on the previous behaviour with `left: [Ref(None)], right: [Int]`. Assisted-by: Claude * jit: port arraylen_vable to the codewriter `rewrite_op_getarraysize` (`jtransform.py:808-817`) is the third consumer of `vable_array_vars`, alongside `rewrite_op_getarrayitem` and `rewrite_op_setarrayitem`. The codewriter had the other two and answered a `len()` over a virtualizable array with a plain `arraylen_gc` on the raw array pointer. Adds `OpKind::VableArrayLen`, the `rewrite_op_getarraysize` arm, and the assembler encoding for the `arraylen_vable/rdd>i` key that `insns.rs`, `blackhole.rs`, `opimpl_arraylen_vable` and `bhimpl_arraylen_vable` already carried. The macro lowering (`majit-macros` `lower_vable_array_len`) emitted the instruction; the codewriter path did not. Assisted-by: Claude * jit: drop the getfield a virtualizable array field read produced `rewrite_op_getfield`'s `except VirtualizableArrayField:` handler ends in `return []` (`jtransform.py:848-857`): registering the base in `vable_array_vars` is the whole rewrite. The port kept the op, so a `getfield_gc_r` of the array pointer stayed in the jitcode and fell through to the immutability-rank rewrite below. All three consumers now answer against the vable base, so the read has no user left. Assisted-by: Claude * jit: fix indentation and the descr-pair citation on the arraylen_vable arms Two of the new match arms landed at the wrong column; `cargo fmt` leaves them alone because it bails on the enclosing `match` in both files. The descr-pair comments named `expect_matching_vable_array_descrs`, which is `pyre-jit`'s assembler. The runtime that decodes the emitted `arraylen_vable/rdd>i` is `MIFrame::vable_array_index_pair_at`. Assisted-by: Claude * jit: address the #1374 review — gate the vable arms, and peel one reference `rewrite_op_getfield` runs whether or not `lower_virtualizable` is set, because the quasi-immutable tail below it does not depend on virtualizable lowering. Its two virtualizable arms do. With `vable_arrays` set and the flag off, the array arm registered a base no consumer would read and dropped a read those consumers still referenced, leaving regalloc an undefined variable. `strip_ty_wrappers` peels `Ref` repeatedly, so an iterator over `&[&i64]` recorded its `Option<&&i64>` payload as `Int` and put a pointer in the integer register bank. `iterator_payload_element` peels the one reference the iterator adds and leaves the element's own. Also: require `array_lengths.len() == array_field_offsets.len()` rather than non-emptiness, since the zip truncates a short vector silently; pin the `arraylen_vable/rdd>i` wire shape and its descr pair; and fail the unroll_safe inventory when two harvested paths share a leaf, which is the assumption it matches on. Assisted-by: Claude * jit: read the iterator ADT before peeling, and catch vable array escapes by any route `iterator_payload_element` peeled one reference off every `next()` payload. A slice iterator adds that reference, but the by-value iterators `is_concrete_iter_constructor` admits do not: `alloc::vec::into_iter::IntoIter` and `core::array::iter::IntoIter` yield `Option<T>`, so the payload is the element already. `Vec<&i64>` and `[&i64; N]` therefore recorded their `&i64` element as `Int` and put a pointer in the integer register bank -- the mirror image of the `&[&i64]` defect the peel was added for. The `next()` receiver names the iterator ADT; peel only for `core::slice::iter::Iter` / `IterMut`. `slice_of_refs_sum` and `array_of_refs_sum` carry both shapes in the corpus. Peeling unconditionally fails the first, never peeling fails `branch_loop_sum_next_yields_an_int_element`; no fixed answer passes both. `check_no_vable_array` enumerated four operand positions. Registering a variable in `vable_array_vars` drops the `getfield` that defined it, and nothing prunes dead operations between `transform` and regalloc, so any operand a kept operation still names is a variable used and never defined. A fifth route scans every operand of every operation the block kept; it reports last and least precisely, and it exists because the four are an enumeration. `_handle_list_call` carries no `vable_array_vars` check and is owed none: upstream splits on `resizable`, putting the check on the `do_fixed_list_*` arms whose receiver is a `GcArray`, and every spelling pyre ports is of the resizable family with a `W_ListObject` receiver. Also: decode the assembled bytes in the `arraylen_vable` wire-shape test rather than only the descr pool order; exercise `lower_virtualizable = false` on the scalar-field arm as well as the array arm; and run the unroll_safe leaf-collision check after the CONTROL guard, so a stale artefact is skipped rather than judged. Assisted-by: Claude * majit: cover the untested virtualizable abort paths `VableArrayIndexNotConcrete` and `GuardSnapshotVableUntyped` had no tests. Neither fires on the synth corpus (0 across the 374 fixtures that trace, where `VableEscapedDuringResidualCall` takes 123). - `array_vable_handlers_with_unpinned_index_surface_index_not_concrete` drives `getarrayitem_vable_i` / `setarrayitem_vable_i` with a seeded vable ref and an index register holding no concrete value. - `an_untyped_virtualizable_box_is_not_snapshot_buildable` pins `TraceCtx::vable_snapshot_buildable` over an absent box list, an all-typed list, and an untyped entry in each of the two positions `build_vable_snapshot_boxes` reads separately. - `build_vable_snapshot_boxes_panics_on_an_untyped_{identity,entry}` pin the two `.expect()` calls that predicate keeps unreachable. Assisted-by: Claude * majit: drop the optimizer's virtualizable array-element seeding Both loop-close arms carry the tracer's live `virtualizable_boxes` shadow into the JUMP as `[reds..., virtualizable_boxes[..-1]]`: the macro state-field JIT through `JitState::collect_jump_args_with_boxes`, PyFrame through `jitcode_dispatch::append_virtualizable_boxes`. PyFrame reaches the second only — nothing under `pyre/` produces `TraceAction::CloseLoop`, so its `collect_jump_args_with_boxes` override is not called in production; note that where the override is defined. `elements_carried_via_shadow` classified PyFrame as not shadow-carried and kept `track_array_elements` on for it, so `VirtualizableTracker::init` seeded element state from the trace-entry input args. Remove that seeding, along with `VirtualizableConfig::track_array_elements`, `::array_lengths` and the length patch in `current_virtualizable_optimizer_config`. The standard-path read answers from the shadow and records no op (`vable_getarrayitem_*_checked`, pyjitpl.py:1170-1184), and the tracer updates the shadow through `set_virtualizable_entry_at` without recording one, so a seeded element box had nothing to fold against and could go stale. Measured before removal: check.py dynasm 434/434, zero jit-stats counters moved. Replace the three tests that pinned the removed length assertion with one that pins what `ensure_setup` still owes — the identity `PtrInfo::Virtualizable` install — and state in the tracker's doc which parts remain and what retiring them would require. Also check `set_virtualizable_entry_at`'s documented precondition against `virtualizable_slot_type` instead of only stating it: a non-Ref value in a Ref slot decodes to NULL through `value_as_ref_bits`. Assisted-by: Claude
&*(*p).fwas classified as taking the address of(*p).f. It reads thevalue that field holds. Every
locals_w!read of the frame's virtualizablearray has that shape, so the classification kept all of them out of the
virtualizable protocol — and that is what stopped
@jit.unroll_safefrombeing portable onto
PyFrame::fast2locals.The defect
build_rvalue'sRvalue::RefandRvalue::RawPtrarms testedmatches!(&place.kind, PlaceKind::Projection(..)). That is true for both&(*p).fand&*(*p).f; only the first names a field's address.The mark is not inert.
mark_place_address_ofsetstaken_by_addresson thedescriptor of the last op the projection emitted — the
getfield.rewrite_op_getfieldfoldssuppresses_virtualizable()intofresh_virtualizable, and thevable_array_varsinsert is gated on!fresh_virtualizable:So a wrongly marked read is never registered. It stays an ordinary
getarrayitem_gcagainst the heap array — not a slower equivalent of theprotocol, a stale read: the heap array is synchronised only at
sync_virtualizable_before_jit,sync_virtualizable_after_jitandsync_virtualizable_after_guard_failure, andVirtualizableInfo::to_optimizer_configpassesarray_lengths: vec![], sothe optimizer's const-index backstop is unseeded too.
locals_w!expands to&*$frame.locals_cells_stack_w. It has 44 callsites across
pyframe.rs(27),eval.rs(15) andbuiltins.rs(2).The fix
place_ref_is_address_ofreturns false when the outermost projection elementis
Deref. The test is spelled the wayresolve_placeandemit_projection_writealready spell it, applied one level out;mir.rshasfive other sites with the identical
PlaceKind::Projection(_, ProjectionElem::Atom(s)) if s == "Deref"match.It is deliberately narrow.
addr_of_mut!(frame.locals_cells_stack_w)isField-last and stays marked —
address_of_the_vable_array_slot_is_marked_not_a_readpins exactly that, including its own anti-vacuity guard.
@jit.unroll_safeonfast2localsUpstream
pyframe.py:572decorates it.look_inside_graphcancelscontains_loopfor a graph carrying the hint (policy.py:61-62), so the slotloop stops keeping the codewriter out.
Ordering is load-bearing and this PR respects it. The hint is the arming
switch for the defect, not a consumer of it: it admits
fast2localsinto thejitcode population, which is what makes the wrongly-classified reads reachable.
Ported alone it is either dead or latently wrong. The defect fix is the first
commit here; the port is the second.
fresh_virtualizableis not an escape hatch for a reader that trips_check_no_vable_array. Upstream'sis_virtualizable_getsetreturns False onthat flag before
raise res, so hinting a reader removes the access from theprotocol rather than fixing it — i.e. it silences the build by shipping the
stale read.
rlib/jit.py:90documents it as "virtualizable was justallocated", and upstream's only production use is
PyFrame.__init__.The escape the fix exposed, and why the second commit exists
Applying the first commit alone breaks the build. That is not a
speculation —
cargo check -p pyre-jit-tracedied with:Registering 44 previously-suppressed reads arms
_check_no_vable_arrayforevery graph holding one, and
peek_atwas the first to trip it.Lowering it says why, and the contrast with a sibling isolates the cause:
A subscript evaluates its receiver before its index expression, so
locals_w!(self)[self.valuestackdepth - 1 - depth]emits the array readfirst and the two subtractions' overflow checks after it; each check branches,
and the array rides the links.
peekvalue_maybe_nonecomputes the index intoa local first, which is also how
pyframe.py:479-484spells it, and its readand use land in one block. The slice bounds check is not the problem — the
front folds it into the
ArrayRead, which is exactly what that contrastshows.
Six sites spelled the arithmetic inside the brackets; the other twenty-four
already hoisted it. Loop-shaped readers (
clear_stack_above,peekvalues,restore_resume_state_from,build_snapshot_frame) are residualized bycontains_loopand never reach the check — which is the same reason theunroll_safehint is the arming switch forfast2locals.Verification
cargo check -p pyre-jit-traceis the authoritative gate:build.rscallsgenerate_intowith nocatch_unwind, so an escaping graph is a hard buildfailure rather than a degraded trace, and CI runs it via
cargo test --all.With all three commits applied, against a freshly extracted and stamped set of
all four artefacts (
--checkclean,window writes: 0 candidate(s)):_check_no_vable_arraypanic;fast2localsis among them — the port takes effect;peek_atandpeekstill in the population, now without escaping.Tests, all green:
a_deref_last_projection_reads_a_value_rather_than_naming_an_addresspinsthe classification on five constructed shapes with no LLBC, so it runs in
ordinary CI.
test_vable_array_len.rs, includingaddress_of_the_vable_array_slot_is_marked_not_a_read—addr_of_mut!isField-last and must stay marked, so this is the over-reach guard for commit
1 — and
every_vable_array_read_in_fast2locals_is_consumed_in_its_block,which covers exactly the graph commit 3 admits.
test_fast2locals_codewriter.rs, both tests.Follow-ups (not in this PR)
VirtualizableInfo::to_optimizer_configpassesarray_lengths: vec![],leaving
optimize_getarrayitem_gc'stracked_array_elementunseeded.baseobjspace.rs:13921quotes upstream_unpackiterable_known_length_jitlookwith its
@jit.unroll_safein a doc comment, but carries no attribute.front/iter_next.rs::is_iter_op_segmentsadmits non-GC slices (&[usize],&[u8]); narrowing it needs the container's element type.Summary by CodeRabbit
Bug Fixes
Performance and Stability